TEL-730: Adding name-addr validation#1650
Conversation
🦋 Changeset detectedLatest commit: 3bc65de The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
alexlivekit
left a comment
alexlivekit
left a comment
There was a problem hiding this comment.
We definitely don't want to start off with strict erroring. This will allow us to at least log errors and understand the frequency and impact of something like this before customer start seeing failures.
| } | ||
| } else { | ||
| // This is a bare URI, and should comply with addr-spec, no special characters | ||
| if strings.ContainsAny(value, ";,? ") { |
There was a problem hiding this comment.
A semicolon here would mean that what's after that is header params, not the URI. However, it is still valid syntax
| if cb := softFailureReporter; cb != nil { | ||
| cb(err) | ||
| } |
There was a problem hiding this comment.
Should we do } else { return err } here? Was considering it.
There was a problem hiding this comment.
why don't we return error when we know the validation failed?
There was a problem hiding this comment.
Because if some customers are using the system with invalid values today, we don't want to cause an outage when this lands. So we "soft-fail" first, allowing us to log the would-be errors for a while before we turn this on.
There was a problem hiding this comment.
not sure how would that be an outage since those calls are already failing.. 🤔
There was a problem hiding this comment.
We would be enabling this check not just for refer messages.
hechen-eng
left a comment
hechen-eng
left a comment
There was a problem hiding this comment.
have some comments, but nothing blocking
2 participants
No description provided.